Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: DH-14630 Added support for null in useTableListener #1227

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

bmingles
Copy link
Contributor

@bmingles bmingles commented Apr 14, 2023

Supports Enterprise DH-14630

resolves #1228

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #1227 (b3fff7c) into main (8ac7dc8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1227   +/-   ##
=======================================
  Coverage   44.25%   44.25%           
=======================================
  Files         450      450           
  Lines       33488    33488           
  Branches     8427     8427           
=======================================
  Hits        14820    14820           
  Misses      18618    18618           
  Partials       50       50           
Flag Coverage Δ
unit 44.25% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/jsapi-components/src/useTableListener.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bmingles bmingles marked this pull request as ready for review April 14, 2023 22:56
@bmingles bmingles requested review from vbabich and mattrunyon April 14, 2023 22:57
@mattrunyon
Copy link
Collaborator

Is this just a nicety from the types perspective? As in most places we would call this we'd need to do useTableListener(table ?? undefined) right now? Or is there a distinct reason for null value?

Looks like there's other hooks (useTable, useTableColumn) that would want the same treatment if that's the case, but the few use cases the table type is Table | undefined

@bmingles
Copy link
Contributor Author

bmingles commented Apr 14, 2023

This just happened to be for a use case I am working on in another PR. Also, it was a chance to add test coverage to this one hook. But yes, this is just a convenience to avoid the coalescing. This also seemed trivial to reason about that it wouldn’t have any unexpected side effects.

@bmingles bmingles merged commit e485c86 into deephaven:main Apr 18, 2023
@bmingles bmingles deleted the web_DH-14630_use-table-listener branch April 18, 2023 15:39
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2023
@bmingles bmingles changed the title feat: Added support for null in useTableListener feat: DH-14630 Added support for null in useTableListener Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DH-14630: useTableListener support null
3 participants